Skip to content
This repository has been archived by the owner on May 5, 2023. It is now read-only.

Adding connection string parser. #376

Merged
merged 27 commits into from Oct 10, 2012
Merged

Conversation

andrerod
Copy link

No description provided.

* @param {number} connectionString The connection string to be parsed.
* @return {object} The query string object.
*/
ConnectionStringParser.parse = function (connectionString) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it works, but it feels really weird to see assignment to a property of a variable before that variable has been defined. Could you move the definition of the constructor function up above the definitions of the methods?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this some more, I don't see any reason for this to be a "static member" of ConnectionStringParser - just make it a top level function in the module.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's 1 reason... styling. Makes it more coherent with other languages and modules.

@christav
Copy link

Is there a bug# for this work?

*/
ConnectionStringParser.prototype._parse = function () {
var self = this;
var parts = this._connectionString.split(';');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there's an escaped ';' or one inside a quoted block? Is this possible? If so, you'll need to handle this differently.

ConnectionStringParser.prototype._skipOperator = function (operatorChar) {
if (this._value[this._pos] != operatorChar) {
// Character was expected.
throw new Error('Character not expected ' + operatorChar);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweak error message to "expecting " + operatorChar + " but instead got " + currentChar + " at position " + this._pos.toString()

Or something like that. Please give more information about what's wrong and where it's wrong.

*/
ConnectionStringParser.prototype._extractString = function (quote) {
var firstPos = this._pos;
while (this._pos < this._value.length && this._value[this._pos] !== quote)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to escape quotes within a quoted string? If so then this needs to be a little more complex to handle that case. If not, you're good.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nop.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrerod
Copy link
Author

Updated.

});

test('parseQuotedValues', function (done) {
var parsedConnectionString = ConnectionStringParser.parse('"test key"=\'value of test\'');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice update on the test cases here and below.

ConnectionStringParser.prototype._skipOperator = function (operatorChar) {
if (this._value[this._pos] != operatorChar) {
// Character was expected.
throw new Error('expecting ' + operatorChar + ' but instead got ' + currentChar + ' at position ' + this._pos);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no currentChar variable - you should probably define one, or replace with the full expression.

andrerod pushed a commit that referenced this pull request Oct 10, 2012
Adding connection string parser.
@andrerod andrerod merged commit a6830e1 into Azure:dev Oct 10, 2012
tjanczuk pushed a commit to tjanczuk/azure-sdk-for-node that referenced this pull request Oct 30, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants